Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix keyword ignores #1236

Merged
merged 2 commits into from
Jun 14, 2020
Merged

Fix keyword ignores #1236

merged 2 commits into from
Jun 14, 2020

Conversation

canyon289
Copy link
Member

Description

hpd forwarding to hdi has a bug where it ignores user arguments

Here's a small example

def function_1(value="a", **kwargs):
    function_2(value="TheWrongVal", **kwargs)

def function_2(value=None, **kwargs):
    print(value)


function_1("a")
function_1("test_val")

Checklist

  • Follows official PR format
  • Includes new or updated tests to cover the new feature
  • Changes are listed in changelog

@canyon289
Copy link
Member Author

Note to self. I also need to add a test to ensure HDI values are being calculated to correct values, not just some value

@OriolAbril
Copy link
Member

Should we make a patch release for this? When are we planning to remove hpd?

@canyon289
Copy link
Member Author

We probably should do patch because it breaks existing functionality, unless we are removing hpd and making a breaking change soon

@codecov
Copy link

codecov bot commented Jun 11, 2020

Codecov Report

Merging #1236 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1236   +/-   ##
=======================================
  Coverage   93.13%   93.13%           
=======================================
  Files          95       95           
  Lines        9614     9614           
=======================================
  Hits         8954     8954           
  Misses        660      660
Impacted Files Coverage Δ
arviz/stats/stats.py 96.20% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34c3b13...2a426a6. Read the comment docs.

@OriolAbril
Copy link
Member

Releasing a patch sounds good, just wanted to check. Should it be done on current master or adding only this commit to previous patch release? Not sure if html repr and other brand new and not much tested functionality should go in the patch, none of this is breaking but it is really new.

@canyon289
Copy link
Member Author

Oh man this is a good point. Maybe we just cut new release then in shorter timeframe. Will think on it today reply here

@canyon289
Copy link
Member Author

Alright i dont think a more complciated branching structure is worth it. I think we merge and make the next release, but I also wont do it until governance is over because there's a bunch going on with that, so 3 weeks or so from me.

If someone else wants to cut a release though I support it

@canyon289 canyon289 merged commit 9b8e29d into arviz-devs:master Jun 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@canyon289 @OriolAbril